Skip to content

#1404 Added Message History to Serial Monitor #1562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 25, 2022

Conversation

nmzaheer
Copy link
Contributor

@nmzaheer nmzaheer commented Oct 16, 2022

Motivation

Feature added for message history in Serial Monitor

Change description

Utilizes the RingList implementation of @dwightfowler-cd here with some corrections for traversal of the list

Other information

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@nmzaheer nmzaheer changed the title Added Message History to Serial Monitor #1404 Added Message History to Serial Monitor Oct 16, 2022
@nmzaheer
Copy link
Contributor Author

There was an error in the next method wherein the last value was being shown twice. It has been corrected with the latest commit. I have also downloaded the artifact and tested against it.

@kittaakos
Copy link
Contributor

Thank you, @nmzaheer. I will take a look soon. Can you tell me something about the expected behavior? Previously (#1467), the stack was named as a ring, now it's a list. Shall I assert the monitor changes compared with IDE 1.x message history behavior?

@kittaakos kittaakos self-requested a review October 17, 2022 15:51
@nmzaheer
Copy link
Contributor Author

I had started with the code in #1467 . The discussion there was about renaming the RingList to a different name as Ring implies cyclical behavior. Although HistoryStack was suggested as an alternative, I believed that was wrong. The functionality required traversal through the data structure and hence HistoryList was better name,

The expected behavior should be as in Arduino 1.x . Let me know once you have tested it.

@kittaakos kittaakos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: serial monitor Related to the Serial Monitor labels Oct 19, 2022
@dwightfowler-cd
Copy link
Contributor

dwightfowler-cd commented Oct 19, 2022 via email

@kittaakos
Copy link
Contributor

Is the team getting close to a release?

I will ensure this feature is part of the next patch release of IDE2. We plan to roll out the next version this month. Please bear with us.

@dwightfowler-cd, if you do not plan to do follow-ups on #1467, please close it. Thanks for keeping the repo clean ❤️

@nmzaheer
Copy link
Contributor Author

@kittaakos Did you have a chance to test the behaviour? Do you have any feedback for me?

@kittaakos
Copy link
Contributor

It's working great. Thank you for your contribution. I believe the logic can be simplified. Let's remove the unnecessary state from the HistoryList. See my proposal here. What do you think?

Please give it a try. If you like it, you can cherry-pick the changes, or you can add my fork to your remotes and pull in the changes.

@kittaakos kittaakos added the status: changes requested Changes to PR are required before merge label Oct 20, 2022
@nmzaheer
Copy link
Contributor Author

nmzaheer commented Oct 20, 2022

Thank you for testing. Your implementation looks much neater and simpler than what I had submitted.

I am testing your implementation of HistoryList with this code. However, it skips the first entry when next method is run. I will have to correct the code accordingly.

I'll get back to you with the changes.

Edit
I think the below code implementation is more apt since the previous method should not return an empty string when it has reached the beginning of the list. In IDE 1.x, once the index reaches the beginning of the list, the first element is returned always.

  previous(): string {
    if (this.index === -1) {
      this.index = this.items.length - 1;
      return this.items[this.index];
    }
    if (this.hasPrevious) {
      return this.items[--this.index];
    }
    return this.items[this.index];
  }

Looking forward to your feedback.

- The implementation has been taken from @kittaakos repo
https://github.com/kittaakos/arduino-ide/blob/d10de017362989b62d01991a33eaef9e71a6aec4/arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
- The previous method has been modified to ensure the first element instead of an empty string is returned if the index is at the beginning of the list.
- The push method has been modified to check if the current command is same as the last command. If same then, it is not added to the list else it is added.
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is looking good and the monitor history works as in IDE 1.x 👍

@kittaakos kittaakos requested a review from per1234 October 21, 2022 07:10
@kittaakos kittaakos self-requested a review October 21, 2022 07:34
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It's looking good.

@kittaakos kittaakos removed the status: changes requested Changes to PR are required before merge label Oct 21, 2022
@kittaakos kittaakos merged commit b837068 into arduino:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself topic: serial monitor Related to the Serial Monitor type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serial Monitor message history
3 participants